Skip to content

Ignore forked but not exec'd child processes#1302

Merged
blt merged 5 commits intomainfrom
blt/ignore_forked_but_not_exec_d_child_processes
Mar 31, 2025
Merged

Ignore forked but not exec'd child processes#1302
blt merged 5 commits intomainfrom
blt/ignore_forked_but_not_exec_d_child_processes

Conversation

@blt
Copy link
Copy Markdown
Collaborator

@blt blt commented Mar 27, 2025

What does this PR do?

This commit introduces code into the procfs Sampler such that discovered
children that are not exec'd yet are not polled. We have found that in
rare cases this causes a double-count of the parent's memory, although it's
not accumulated as such by Linux itself.

This commit introduces code into the procfs Sampler such that discovered
children that are not exec'd yet are not polled. We have found that in
rare cases this causes a double-count of the parent's memory, although it's
not accumulated as such by Linux itself.

Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
Copy link
Copy Markdown
Collaborator Author

blt commented Mar 27, 2025

@blt blt marked this pull request as ready for review March 27, 2025 18:39
@blt blt requested a review from a team as a code owner March 27, 2025 18:39
Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
// This is a new process. We initialize its
// process info and then determine by
// examination of the exe/cmdline if the process
// is not exec'd -- meaning we will not poll it
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// is not exec'd -- meaning we will not poll it
// is fork'd but not exec'd -- meaning we will not poll it

let parent_info = match initialize_process_info(parent_pid).await {
Ok(Some(info)) => info,
Ok(None) => {
warn!("Could not initialize parent process info");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going back and forth on whether this is fatal or not, and I agree that this is fine to return Ok, if we return an Err, then the observer server will crash and this will never retry, which is not the behavior we want. This is a retry-able error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To your point, this is an error and not a warning.

Technically lading recovers but an error has happened that results in unexpected results.

blt added 2 commits March 27, 2025 16:45
Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
Copy link
Copy Markdown
Collaborator Author

blt commented Mar 31, 2025

Merge activity

  • Mar 31, 2:37 AM EDT: A user started a stack merge that includes this pull request via Graphite.
  • Mar 31, 2:38 AM EDT: A user merged this pull request with Graphite.

@blt blt merged commit d4dbca4 into main Mar 31, 2025
22 checks passed
@blt blt deleted the blt/ignore_forked_but_not_exec_d_child_processes branch March 31, 2025 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants